Skip to content

Gate exception field serialization behind langversion 11#19746

Open
T-Gro wants to merge 5 commits into
mainfrom
feature/exception-serde-langversion
Open

Gate exception field serialization behind langversion 11#19746
T-Gro wants to merge 5 commits into
mainfrom
feature/exception-serde-langversion

Conversation

@T-Gro
Copy link
Copy Markdown
Member

@T-Gro T-Gro commented May 15, 2026

Gates the GetObjectData + field-restoring ctor from #19342 behind LanguageFeature.ExceptionFieldSerializationSupport (F# 11).

With langversion ≤10 (current default), exception codegen is unchanged from pre-#19342 behavior.

@T-Gro T-Gro requested a review from a team as a code owner May 15, 2026 12:26
@T-Gro T-Gro requested a review from abonie May 15, 2026 12:26
@T-Gro T-Gro added AI-Auto-Resolve-Conflicts Opt-in: LabelOps merges main into this PR and resolves conflicts every 3h AI-Auto-Resolve-CI Opt-in: LabelOps triages CI failures on this PR every 3h labels May 15, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 15, 2026

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

…FieldSerializationSupport (F# 11)

The GetObjectData override and field-restoring deserialization constructor
for exception types are now gated behind langversion 11. With langversion
<=10 (the current default), exceptions emit only the base-call ctor
(status quo ante PR #19342).

- Add LanguageFeature.ExceptionFieldSerializationSupport, mapped to F# 11.0
- Gate shouldRestoreFields and GetObjectData emission on the feature
- Update tests to use withLangVersion "11"
- Update all IL baselines (SerializableAttribute, Nullness) to reflect
  the default-langversion output (no field serde IL)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro T-Gro force-pushed the feature/exception-serde-langversion branch from 16c595f to 733c88b Compare May 15, 2026 12:27
@github-actions github-actions Bot added the AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed label May 15, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

Commit pushed: 4ee810c

Generated by LabelOps — PR Maintenance

@github-actions

This comment has been minimized.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

Commit pushed: 1a04573

Generated by LabelOps — PR Maintenance

@github-actions

This comment has been minimized.

@github-actions github-actions Bot mentioned this pull request May 16, 2026
The net472 baseline was incorrectly updated to use [runtime] prefixed
NullableContextAttribute and NullableAttribute references. On net472,
these attributes are embedded in the assembly (not from runtime BCL),
so they must not have the [runtime] prefix. Also restore the embedded
attribute class definitions at the end of the file.

The serialization changes (removing field-restoring ctor and GetObjectData)
are correctly kept, as they match the default langversion behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

Commit pushed: f1a9d69

Generated by LabelOps — PR Maintenance

@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Member Author

@T-Gro T-Gro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Gate exception field serialization behind langversion 11

This is a well-structured PR that properly gates the behavioral change from #19342 behind LanguageFeature.ExceptionFieldSerializationSupport (F# 11). The approach is sound — emitting GetObjectData and the field-restoring deserialization ctor changes the binary contract of exception types, so gating it is the right call for backward compatibility.

What's good

  • Clean gating logic: The emitFieldSerialization boolean at line 12320-12323 is easy to follow and correctly combines langversion check, FSharp.Core exclusion, and empty-fields short-circuit.
  • Test coverage: The runtime roundtrip tests and the MessageFieldCollision regression test explicitly opt into langversion 11. The FSharp.Core negative test (Issue_878_FSharpCoreExceptions_NoGetObjectDataOverride) correctly validates that FSharp.Core exceptions are still excluded.
  • Baseline IL updates: All baselines that compile at default langversion correctly no longer contain GetObjectData / field-restore IL — this proves the gate works for the default path.
  • Release notes: Properly updated to mention the langversion gate.
  • Code structure improvement: Moving ilInstrsToSaveFields, ilInstrsForGetObjectData, and ilGetObjectDataDef inside the if emitFieldSerialization then branch avoids constructing those values when they'll never be used.

Minor observations (non-blocking)

  1. ilInstrsToRestoreFields still computed unconditionally (lines 12286-12318): Unlike the save/GetObjectData side which was moved inside the if, the restore instructions are computed even when emitFieldSerialization = false. The list is just discarded via the if ... then ilInstrsToRestoreFields else [] at line 12332. Consider moving it inside the branch for symmetry and to avoid the allocation in the common (langversion < 11) path. Not a correctness issue.

  2. No explicit negative test: The IL baselines implicitly prove that default langversion doesn't emit field serialization, but an explicit [<Fact>] that compiles an exception at langversion 10 and asserts absence of GetObjectData would be stronger documentation of intent. The existing coverage via Nullness/SerializableAttribute baselines is adequate though.

Overall: clean, minimal, correct.

@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label May 18, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

Commit pushed: 8281f57

Generated by LabelOps — PR Maintenance

@github-actions
Copy link
Copy Markdown
Contributor

🤖 LabelOps — CI fix.

Added release notes entry to docs/release-notes/.FSharp.Compiler.Service/11.0.100.md to fix the check_release_notes check failure.

The fsharp-ci overall failure was caused by MacOS Batch2 being cancelled (all individual test jobs passed successfully). This should resolve on the next CI run.

Generated by LabelOps — PR Maintenance · ● 30.6M ·

@T-Gro T-Gro added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-Auto-Resolve-CI Opt-in: LabelOps triages CI failures on this PR every 3h AI-Auto-Resolve-Conflicts Opt-in: LabelOps merges main into this PR and resolves conflicts every 3h AI-reviewed PR reviewed by AI review council AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

1 participant